Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for configuring multiple OH targets #1900

Merged
merged 44 commits into from Sep 20, 2020

Conversation

maniac103
Copy link
Contributor

Only one can be active at a time, though. Selection of targets happens via toggle in the navigation drawer.

Closes #10

@maniac103
Copy link
Contributor Author

This is mostly finished, but the configuration UI needs more work (e.g. confirmation prompts when leaving without saving or deleting servers; also some sort of error indication on incomplete server configurations).
Before progressing, I wanted to collect feedback on the UI aspects of this first.

@mueller-ma
Copy link
Member

mueller-ma commented Mar 27, 2020

I few UI suggestions:

  • In demo mode hide the server selector in the drawer.
  • "Show all sitemaps in drawer setting":
    If enabled I'm fine with the current behavior. If disabled, I suggest the following menu structure (with servers A and B):
Server A (opens default sitemap of server a)
Server B (opens default sitemap of server b)
<Divider>
<Server selector>
HABPanel
Notifications
NFC
<Divider>
Settings
About

The default sitemaps of all servers are always directly selectable, but HABPanel, notifications and NFC only from the selected server.

  • If HABPanel, notifications or NFC are open and the user changes the server, the current page should be updated, e.g. show HABPanel from new server.
  • Update the title of the HABPanel fragment to HABPanel <server name>? Same for notifications and NFC.

Things that need to be updated for multi server support:

  • Default sitemap: Do the same than for username, password and url (save on per id basis).
  • Sitemap shortcuts: Save the server id with the shortcut. Users can adjust the label by themselves (if the launcher supports that).
  • Widgets: Save the server id in the widget data
  • Background tasks: Just send them to all servers?
  • NFC tags: That's more tricky as the server id will be different on different devices. Save the local url on the tag (the remote one might not be unique)? If no url is present, use connection 0. If the url doesn't match any configured url, show a toast.
  • Server discovery on first start: The log says "Service resolved: ....", but the loading indicator keeps spinning. 30 seconds after the first server has been found my second one was found. So maybe let it search for a minute or so and save all found servers?

And a small feature request:

  • Add a setting to the local connection called wifi ssid. When the app is opened, the active server is selected based on the wifi ssid, if configured.

@mueller-ma mueller-ma added the enhancement Indicates new feature requests label Mar 27, 2020
@maniac103
Copy link
Contributor Author

In demo mode hide the server selector in the drawer.

Makes sense.

"Show all sitemaps in drawer setting":
If enabled I'm fine with the current behavior. If disabled, I suggest the following menu structure (with servers A and B):

I really don't want to move UI controls based on some (from a user POV) unrelated setting. Besides the (IMHO unnecessary) complication it feels just inconsistent and 'magic' to me.

If HABPanel, notifications or NFC are open and the user changes the server, the current page should be updated, e.g. show HABPanel from new server.

This seems questionable to me, since I don't think it's clear the user wants to check notifications on server B after he just checked server A. What does Gmail (or other Google app) do in that case?

Update the title of the HABPanel fragment to HABPanel ? Same for notifications and NFC.

IMHO unneeded and clutter since the current server name is readily available in the drawer.

Default sitemap: Do the same than for username, password and url (save on per id basis).

Indeed.

Sitemap shortcuts: Save the server id with the shortcut. Users can adjust the label by themselves (if the launcher supports that).

Right, but what to do with existing ones, and what to do with shortcuts belonging to a server when that server is removed?

Widgets: Save the server id in the widget data

Same as for shortcuts.

Background tasks: Just send them to all servers?

Uh, here it starts getting ugly. Automatically sending it to all servers may be a privacy concern, but I don't see how it should work otherwise.

NFC tags: That's more tricky as the server id will be different on different devices. Save the local url on the tag (the remote one might not be unique)? If no url is present, use connection 0. If the url doesn't match any configured url, show a toast.

Connection 0 should be 'active server', but yeah, makes sense.

Server discovery on first start: The log says "Service resolved: ....", but the loading indicator keeps spinning. 30 seconds after the first server has been found my second one was found. So maybe let it search for a minute or so and save all found servers?

I would make it stop on the first found server as it's always been the case. If the loading indicator doesn't go away, that's a bug.

Add a setting to the local connection called wifi ssid. When the app is opened, the active server is selected based on the wifi ssid, if configured.

Maybe in a later PR ... I'd try to keep it simple for now.

@mueller-ma
Copy link
Member

I really don't want to move UI controls based on some (from a user POV) unrelated setting. Besides the (IMHO unnecessary) complication it feels just inconsistent and 'magic' to me.

IMO the drawer should always show only one sitemap and IMO if that's the case, there is enough space for the default sitemaps of the other servers.
However there are users don't like this and want to have all their sitemaps shown in the drawer. This is why I added a preference for that (#1554).
I could also live with the current behavior, if you remove the IMO unneeded divider between the sitemap and HABPanel, if only one sitemap is shown.

Right, but what to do with existing ones, and what to do with shortcuts belonging to a server when that server is removed?

I haven't check yet if it's easily possible to update existing shortcuts and widgets. If so: Add the server id, if not no id == first server. If the server is removed, also remove widgets and shortcuts if possible. If not, just keep them and show a toast if pressed.

Uh, here it starts getting ugly. Automatically sending it to all servers may be a privacy concern, but I don't see how it should work otherwise.

They could be moved to the server preference or simply add a switch to the server prefs with "Send device information to this server" which is enabled by default.

I would make it stop on the first found server as it's always been the case. If the loading indicator doesn't go away, that's a bug.

Probably better. Two servers at the same wifi aren't that common.

Maybe in a later PR ... I'd try to keep it simple for now.

Sure.

@maniac103
Copy link
Contributor Author

I could also live with the current behavior, if you remove the IMO unneeded divider between the sitemap and HABPanel, if only one sitemap is shown.

That we can compromise on :-)

@maniac103
Copy link
Contributor Author

More points:

  • Need to do FCM registration for all servers
  • What server to use for showing registration status?
  • How do we decide which server to go to on notification click?

@mueller-ma
Copy link
Member

A quite simple way would be to have a primary server and make some features for this server only. Then these features could be made multi server compatible in small patches.
The UX would be quite poor however as it may be difficult to know what's multi server compatible and what not.

What server to use for showing registration status?

Show it in the server settings?

How do we decide which server to go to on notification click?

One (summary) notification for each server? Or we show notifications from all servers in the notification fragment. The shown priority could be replaced with the server name.

@maniac103
Copy link
Contributor Author

One (summary) notification for each server?

The thing is, I don't think we can detect what server (or rather login) an FCM message came from.

Or we show notifications from all servers in the notification fragment

Can't meaningfully do that due to the paging.

The more I think about it, and the more features we discover that need adaption, the more I wonder whether all that effort is worth it :-/ I really would be interested how large the part of our user base is who maintains more than one server.

@mueller-ma
Copy link
Member

I really would be interested how large the part of our user base is who maintains more than one server.

I use two openHAB server at different locations.

The more I think about it, and the more features we discover that need adaption, the more I wonder whether all that effort is worth it :-/

Maybe we should really add a pref to set a server as primary and only update a few features, that can be done quite simple (NFC, widgets, shortcuts). If I could view Sitemaps of other servers I would be quite satisfied.
The pref summary could explain what "primary server" means.

@mueller-ma mueller-ma marked this pull request as draft April 10, 2020 11:41
@psmedley
Copy link

I also run openhab in two locations. My primary home and also my holiday house. It would be great to get this finalised and included - my current workaround is to install both the 'openhab' and 'openhab beta' app on my phone.

@mueller-ma
Copy link
Member

mueller-ma commented May 9, 2020

@maniac103 Can I help you with coding for the PR? IMO it would be fine to

  1. Finish the UI work on the settings (like you said in the first comment)
  2. Add a pref for the primary server with a short explanation (saying that the secondary servers only support viewing the sitemaps) and a link to the docs with a list of features that are multi server ready and those that aren't ready.
  3. Mark the multi server support as beta, e.g. by adding "(beta)" to the "Add server" pref
  4. Fix default sitemap selection
  5. Fix discovery
  6. Merge this PR

After that the missing features can be ported to be multi server compatible in separate PRs. For people having multiple servers, but are connected most of the time with their primary one, just viewing sitemaps is good enough.
The "Add server" pref could also be hidden in stable releases.

mueller-ma added a commit to mueller-ma/openhab.android that referenced this pull request Jul 21, 2020
* Add confirmation dialog for deletion
* Mark 'add server' pref as beta
* Fix default sitemap selection
* Other small improvements

Fixes #10
Closes openhab#1900

Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
@tcgerhard
Copy link

Another dual-home Openhaber here. A small twist -- the two sites are connected by a site-to-site VPN. My current workarounds:

  1. a few "important" items shadowed from site2 to site1 via MQTT
  2. Android app uses the credentials for site1; for site2, use habpanel in Chrome browser on the phone (which is a lousy UX)
    I'd be thrilled to have a way to switch credentials easily without manually updating them when switching context.

mueller-ma added a commit to maniac103/openhab.android that referenced this pull request Jul 24, 2020
* Add confirmation dialog for deletion
* Mark 'add server' pref as beta
* Fix default sitemap selection
* Other small improvements

Fixes openhab#10
Closes openhab#1900

Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
@mueller-ma mueller-ma force-pushed the multi-server branch 2 times, most recently from c54705a to 66eb52e Compare July 24, 2020 08:04
@mueller-ma
Copy link
Member

Add a pref for the primary server with a short explanation (saying that the secondary servers only support viewing the sitemaps) and a link to the docs with a list of features that are multi server ready and those that aren't ready.

Currently ConnectionFactory can only handle one server at a time, so it has to be updated to handle at least two servers, primary and active. Example: The app can be open and show the sitemaps of server 2, while some background tasks sends an item update to server 1.

mueller-ma added a commit to maniac103/openhab.android that referenced this pull request Jul 24, 2020
* Add confirmation dialog for deletion
* Mark 'add server' pref as beta
* Fix default sitemap selection
* Other small improvements

Fixes openhab#10
Closes openhab#1900

Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
@mueller-ma mueller-ma force-pushed the multi-server branch 2 times, most recently from 6ea813d to 159a865 Compare July 24, 2020 08:55
@maniac103
Copy link
Contributor Author

Currently ConnectionFactory can only handle one server at a time, so it has to be updated to handle at least two servers, primary and active. Example: The app can be open and show the sitemaps of server 2, while some background tasks sends an item update to server 1.

I'll look into that.

@maniac103
Copy link
Contributor Author

Currently ConnectionFactory can only handle one server at a time, so it has to be updated to handle at least two servers, primary and active. Example: The app can be open and show the sitemaps of server 2, while some background tasks sends an item update to server 1.

I'll look into that.

Done. Questions I came across when implementing this:

  • Should AbstractItemPickerActivity fetch items and icons from active or primary server?
  • When opening a cloud notification via clicking it, how do we want to handle that? Change active server to primary one? Just open primary notifications (I've currently done that)? If the latter, do we want to change the title?
  • I think we'd need some sort of 'unsaved changes reminder' in the server editor fragment. Either as a clear reminder pref, or as a dialog invoked when going back with unsaved changes.

@mueller-ma
Copy link
Member

Should AbstractItemPickerActivity fetch items and icons from active or primary server?

For now the items should be fetched from the primary server. I'm going to adapt AbstractItemPickerActivity for multi server support, but keep the possibility to display the items from the primary server only.

When opening a cloud notification via clicking it, how do we want to handle that? Change active server to primary one? Just open primary notifications (I've currently done that)? If the latter, do we want to change the title?

I wouldn't change the active server, but change the title.

I think we'd need some sort of 'unsaved changes reminder' in the server editor fragment. Either as a clear reminder pref, or as a dialog invoked when going back with unsaved changes.

Yes, that's definitely needed. I'd add a dialog when trying the exit the fragment with unsaved changes.

mueller-ma added a commit to mueller-ma/openhab.android that referenced this pull request Jul 27, 2020
* Add confirmation dialog for deletion
* Mark 'add server' pref as beta
* Fix default sitemap selection
* Other small improvements

Fixes #10
Closes openhab#1900

Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
mueller-ma and others added 24 commits September 19, 2020 12:15
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: Danny Baumann <dannybaumann@web.de>
Signed-off-by: mueller-ma <mueller-ma@users.noreply.github.com>
@psmedley
Copy link

Thanks for all the work on this, looks good to me with my limited testing

@cwihne
Copy link

cwihne commented Sep 28, 2020

Equipped family members in DE and US with individual OH instances, aside of my own installation with >100 things + >1.300 items. Up until now I used released app for remote OHs (reconfiguring connections) + beta app for my system. What a great achievement to have multiple OHs in a single app supported. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support multiple openhab server
5 participants